bake(dev): plugins in dev server, with other fixes #15467

paperdave · 2024-11-28T03:49:58Z

load plugins in dev mode
pass resolveDir to onResolve plugins
onLoad plugin with css loader now works
hmr instrumentation for export { x } from 'y';
fixed export { x as y }
throw SyntaxError The requested module '${id}' does not provide an export named '${key}'
stub import.meta.hot, implement import.meta.hot.dispose

example project: https://github.com/paperdave/bake-svelte-plugin-example

robobun · 2024-11-28T03:50:08Z

❌ @Jarred-Sumner, your commit 8fa6db3 has 14 failures in #7119:

test/js/bun/http/bun-serve-static.test.ts - timeout on 🐧 3.20 x64

test/integration/next-pages/test/dev-server.test.ts - 1 failing on 🐧 3.20 x64-baseline

test/integration/next-pages/test/dev-server.test.ts - 1 failing on 🐧 3.20 x64

test/cli/hot/hot.test.ts - timeout on 🐧 22.04 x64-baseline

test/cli/hot/hot.test.ts - timeout on 🐧 3.20 x64

test/js/node/test/parallel/fs-watch-recursive-linux-parallel-remove.test.js - 1 failing on 🐧 20.04 x64

test/js/node/test/parallel/fs-watch-recursive-linux-parallel-remove.test.js - 1 failing on 🐧 3.20 aarch64

test/js/node/test/parallel/fs-watch-recursive-linux-parallel-remove.test.js - 1 failing on 🐧 22.04 x64

test/js/node/test/parallel/fs-watch-recursive-linux-parallel-remove.test.js - 1 failing on 🐧 22.04 x64-baseline

test/js/node/test/parallel/fs-watch-recursive-linux-parallel-remove.test.js - 1 failing on 🐧 3.20 x64-baseline

test/js/node/test/parallel/fs-watch-recursive-linux-parallel-remove.test.js - 1 failing on 🐧 3.20 x64

test/v8/v8.test.ts - 22 failing on 🐧 3.20 aarch64

test/v8/v8.test.ts - 22 failing on 🐧 3.20 x64-baseline

test/v8/v8.test.ts - 22 failing on 🐧 3.20 x64

test/js/node/child_process/child_process.test.ts - 1 failing on 🐧 3.20 aarch64

test/js/node/child_process/child_process.test.ts - 1 failing on 🐧 3.20 x64-baseline

test/js/node/child_process/child_process.test.ts - 1 failing on 🐧 3.20 x64

test/regression/issue/09041.test.ts - 1 failing on 🐧 22.04 x64

test/regression/issue/09041.test.ts - 1 failing on 🐧 20.04 x64-baseline

test/js/bun/http/serve.test.ts - SIGTRAP on 🐧 3.20 aarch64

test/js/bun/http/serve.test.ts - SIGILL on 🐧 3.20 x64-baseline

test/js/bun/http/serve.test.ts - SIGILL on 🐧 3.20 x64

test/js/node/test/parallel/worker-nested-uncaught.test.js - segmentation fault on 🐧 3.20 aarch64

test/js/web/timers/setTimeout.test.js - 1 failing on 🐧 22.04 aarch64

test/cli/install/bun-link.test.ts - 1 failing on 🐧 22.04 x64

test/integration/next-pages/test/next-build.test.ts - 1 failing on 🍎 14 x64

test/integration/next-pages/test/next-build.test.ts - 1 failing on 🐧 22.04 aarch64

test/js/node/dns/node-dns.test.js - 1 failing on 🪟 2019 x64

test/js/web/timers/setInterval.test.js - 1 failing on 🪟 2019 x64

Jarred-Sumner · 2024-11-30T02:08:51Z

src/bake/DevServer.zig

@@ -2034,15 +2045,16 @@ pub fn IncrementalGraph(side: bake.Side) type {
            const dev = g.owner();
            dev.graph_safety_lock.assertLocked();

-            const abs_path = ctx.sources[index.get()].path.text;
+            const path = ctx.sources[index.get()].path;


Is this copy intentional? Can it be a pointer instead? Path is not a small struct

Suggested change

const path = ctx.sources[index.get()].path;

const path = &ctx.sources[index.get()].path;

Jarred-Sumner · 2024-11-30T02:09:49Z

src/bake/bake.zig

+                promise.setHandled(global.vm());
+                // TODO: remove this call, replace with a promise list that must
+                // be resolved before the first bundle task can begin.
+                global.bunVM().waitForPromise(promise);


Jarred-Sumner · 2024-11-30T02:10:23Z

src/bake/bake.zig

@@ -521,6 +583,10 @@ pub const Framework = struct {
        if (renderer == .server and framework.server_components != null) {
            try out.options.conditions.appendSlice(&.{"react-server"});
        }
+        if (mode == .development) {
+            // Support `esm-env` package using this condition.
+            try out.options.conditions.appendSlice(&.{"development"});


i've never heard of this

Jarred-Sumner · 2024-11-30T02:10:56Z

src/bake/hmr-module.ts

@@ -76,7 +91,79 @@ if (side === "server") {
 }

 function initImportMeta(m: HotModule): ImportMeta {


This is client-side, for browsers I assume?

Jarred-Sumner · 2024-11-30T02:12:23Z

src/bun.js/api/JSBundler.zig

            if (source_code_value.isEmptyOrUndefinedOrNull() or loader_as_int.isEmptyOrUndefinedOrNull()) {
                this.value = .{ .no_match = {} };

                if (this.was_file) {
                    // Faster path: skip the extra threadpool dispatch
-                    completion.bundler.graph.pool.pool.schedule(bun.ThreadPool.Batch.from(&this.parse_task.task));
+                    this.bv2.graph.pool.pool.schedule(bun.ThreadPool.Batch.from(&this.parse_task.task));


bv2 implies the existence of bv3 and bv1

is there a better name?

Jarred-Sumner · 2024-11-30T02:15:56Z

src/bundler/bundle_v2.zig

@@ -1783,13 +1835,15 @@ pub const BundleV2 = struct {
                this.graph.heap.gc(true);
            }
        }
-        var log = &load.completion.?.log;
+        const log = this.bundler.log;


Why was this changed? This will crash if the bundler thread writes to logger.Log at the same time as the main thread. When using Bun.build that can happen.

Jarred-Sumner · 2024-11-30T02:16:57Z

src/bundler/bundle_v2.zig

@@ -1846,7 +1922,7 @@ pub const BundleV2 = struct {
                this.graph.heap.gc(true);
            }
        }
-        var log = &resolve.completion.?.log;
+        const log = this.bundler.log;


Ditto. We shouldn't change this unless we're confident there's no thread-safety issue.

Jarred-Sumner · 2024-11-30T02:18:05Z

src/bundler/bundle_v2.zig

                            if (!record.source_index.isValid()) continue;
+                            if (loaders[record.source_index.get()] != .css) continue;


This is an extra pointer lookup, which slightly slows things down. Was the import_record.tag inaccurate?

Jarred-Sumner · 2024-11-30T02:18:27Z

src/bundler/bundle_v2.zig

@@ -2595,7 +2666,7 @@ pub const BundleV2 = struct {
                    import_record.path.pretty = rel;
                    import_record.path = this.pathWithPrettyInitialized(path.*, target) catch bun.outOfMemory();
                    if (entry.kind == .css) {
-                        import_record.tag = .css;
+                        import_record.path.is_disabled = true;


Can you leave a comment explaining this?

Jarred-Sumner · 2024-11-30T02:19:15Z

src/bundler/bundle_v2.zig

                            temp,
                            visits,
                            o,
                            record.source_index,
-                            record.tag == .css,
+                            loaders[record.source_index.get()] == .css,


If we can continue to use record.tag that's one less pointer lookup

Jarred-Sumner · 2024-11-30T02:19:55Z

src/bundler/bundle_v2.zig

+
+        // add a marker for the client runtime to tell that this is an ES module
+        if (ast.exports_kind == .esm) {
+            try stmts.inside_wrapper_prefix.append(Stmt.alloc(S.SExpr, .{


This would ideally use the lazy export AST and then it can choose whichever output format is best.

Jarred-Sumner · 2024-11-30T02:21:06Z

src/js/builtins/BundlerPlugin.ts

@@ -334,12 +335,12 @@ export function runOnResolvePlugins(this: BundlerPlugin, specifier, inputNamespa
  }
 }

-export function runOnLoadPlugins(this: BundlerPlugin, internalID, path, namespace, defaultLoaderId) {
+export function runOnLoadPlugins(this: BundlerPlugin, internalID, path, namespace, defaultLoaderId, isServerSide: boolean) {


Maybe isServerSide could be an enum to clarify whether it means:

RSC

SSR

!browser

Jarred-Sumner · 2024-11-30T02:21:41Z

src/js_parser.zig

                        // This is a re-export and the symbols created here are used to reference
                        for (data.items) |*item| {
                            const _name = p.loadNameFromRef(item.name.ref.?);
-                            const ref = try p.newSymbol(.other, _name);
+                            const ref = try p.newSymbol(.import, _name);


ooh i wonder what bugs this causes

Jarred-Sumner · 2024-11-30T02:23:34Z

src/logger.zig

+    column: i32,
+    /// Number of bytes this location should highlight.
+    /// 0 to just point at a single character
+    length: usize = 0,


yeah this struct wasn't thoughtfully created, it was copypasta from esbuild

Jarred-Sumner · 2024-11-30T02:24:07Z

src/sourcemap/sourcemap.zig

@@ -607,6 +607,9 @@ pub const ParseResult = union(enum) {
                .location = Logger.Location{
                    .file = path,
                    .offset = this.loc.toUsize(),
+                    // TODO: populate correct line and column information
+                    .line = -1,


We need to port the LineColumnTracker abstraction esbuild has

Jarred-Sumner

Some comments. Only blocking comment is the change to reference bundler.log, as I worry it will cause crashes in Bun.build.

Jarred-Sumner · 2024-11-30T03:07:50Z

I’m going to merge this to unblock further progress, but please take a look at the comments when you get a chance.

Co-authored-by: paperdave <[email protected]> Co-authored-by: Jarred Sumner <[email protected]>

paperdave added 7 commits November 26, 2024 14:01

first impl, not stable

bb3948e

alright guys

ae4349b

a bit more, svelte doesnt fully import yet

1f19128

Merge remote-tracking branch 'origin/main' into dave/dev-plugins

52f5f90

update

32c95a7

fix aliased exports

1e18d5c

more work

5c84c76

paperdave and others added 3 commits November 28, 2024 03:51

bun run clang-format

d7abf15

hello

b2a70c2

Merge remote-tracking branch 'origin/main' into dave/dev-plugins

f7fa503

Jarred-Sumner reviewed Nov 30, 2024

View reviewed changes

Jarred-Sumner requested changes Nov 30, 2024

View reviewed changes

Jarred-Sumner marked this pull request as ready for review November 30, 2024 02:26

Merge branch 'main' into dave/dev-plugins

8fa6db3

Jarred-Sumner merged commit 8aa451c into main Nov 30, 2024
66 of 67 checks passed

Jarred-Sumner deleted the dave/dev-plugins branch November 30, 2024 03:07

snoglobe pushed a commit that referenced this pull request Dec 4, 2024

bake(dev): plugins in dev server, with other fixes (#15467)

91d34b9

Co-authored-by: paperdave <[email protected]> Co-authored-by: Jarred Sumner <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bake(dev): plugins in dev server, with other fixes #15467

bake(dev): plugins in dev server, with other fixes #15467

paperdave commented Nov 28, 2024

robobun commented Nov 28, 2024 •

edited

Loading

Jarred-Sumner Nov 30, 2024

Jarred-Sumner Nov 30, 2024

Jarred-Sumner Nov 30, 2024

Jarred-Sumner Nov 30, 2024

Jarred-Sumner Nov 30, 2024

Jarred-Sumner Nov 30, 2024

Jarred-Sumner Nov 30, 2024

Jarred-Sumner Nov 30, 2024

Jarred-Sumner Nov 30, 2024

Jarred-Sumner Nov 30, 2024

Jarred-Sumner Nov 30, 2024

Jarred-Sumner Nov 30, 2024

Jarred-Sumner Nov 30, 2024

Jarred-Sumner Nov 30, 2024

Jarred-Sumner Nov 30, 2024

Jarred-Sumner left a comment

Jarred-Sumner commented Nov 30, 2024

	const path = ctx.sources[index.get()].path;
	const path = &ctx.sources[index.get()].path;

		@@ -76,7 +91,79 @@ if (side === "server") {
		}

		function initImportMeta(m: HotModule): ImportMeta {

		if (!record.source_index.isValid()) continue;
		if (loaders[record.source_index.get()] != .css) continue;

bake(dev): plugins in dev server, with other fixes #15467

bake(dev): plugins in dev server, with other fixes #15467

Conversation

paperdave commented Nov 28, 2024

robobun commented Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jarred-Sumner left a comment

Choose a reason for hiding this comment

Jarred-Sumner commented Nov 30, 2024

robobun commented Nov 28, 2024 •

edited

Loading